Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Factor in X-Forwarded-Host / Forwarded when capturing server.address and server.port #411

Merged
merged 6 commits into from
Oct 19, 2023
Merged

Factor in X-Forwarded-Host / Forwarded when capturing server.address and server.port #411

merged 6 commits into from
Oct 19, 2023

Conversation

trask
Copy link
Member

@trask trask commented Oct 16, 2023

Fixes #408

Changes

Merge requirement checklist

@trask trask force-pushed the forwarded-server-address-port branch from 574349b to dc5bdb0 Compare October 16, 2023 20:50
@trask trask marked this pull request as ready for review October 16, 2023 20:50
@trask trask requested review from a team October 16, 2023 20:50
@trask trask requested a review from a team October 16, 2023 20:54
@trask
Copy link
Member Author

trask commented Oct 16, 2023 •
edited
Loading

moving back to draft, going to see about cleaning up duplication between traces and metrics first gave up on this, can be revisited

currently blocked on #412 (don't want to forget to add the section to the newly introduced copy there) done

@trask trask marked this pull request as draft October 16, 2023 20:57
@trask trask force-pushed the forwarded-server-address-port branch 4 times, most recently from c26fbcf to c71dc34 Compare October 16, 2023 22:21
@trask trask force-pushed the forwarded-server-address-port branch from c71dc34 to bc05ae4 Compare October 16, 2023 22:21
@trask trask changed the title Consider X-Forwarded-Host / Forwarded when capturing server.address and server.port Factor in X-Forwarded-Host / Forwarded when capturing server.address and server.port Oct 17, 2023
@trask trask marked this pull request as ready for review October 17, 2023 15:14
@lmolkova lmolkova mentioned this pull request Oct 18, 2023
Merged
3 tasks
@joaopgrassi
Copy link
Member

Is this really "breaking"?

@pyohannes
Copy link
Contributor

Is this really "breaking"?

I think so. Implementations who populated server.address with the host identifier from the Host header although a X-Forwarded-Host / Forwarded header was available were compliant before this change, but not after.

@trask trask force-pushed the forwarded-server-address-port branch from 3492c4d to 85a97ce Compare October 18, 2023 17:03
@AlexanderWert AlexanderWert merged commit 965c108 into open-telemetry:main Oct 19, 2023
9 checks passed
@mateuszrzeszutek mateuszrzeszutek mentioned this pull request Oct 19, 2023
Merged
@trask trask deleted the forwarded-server-address-port branch October 14, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pyohannes pyohannes pyohannes approved these changes

@joaopgrassi joaopgrassi joaopgrassi approved these changes

@lmolkova lmolkova lmolkova approved these changes

@jsuereth jsuereth jsuereth approved these changes

Assignees

@jsuereth jsuereth

@AlexanderWert AlexanderWert

Labels
None yet
Projects
Semantic Conventions Triage
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider X-Forwarded-Host / Forwarded when capturing server.address and server.port
6 participants
@trask @joaopgrassi @pyohannes @jsuereth @lmolkova @AlexanderWert